feat(web-ui): /sessions/[id] detail page — SplitPane + AgentChatPanel + AgentTerminal (#509)#520
Merged
Merged
Conversation
- New page wiring SplitPane + AgentChatPanel + AgentTerminal - Header: back link, short session ID, state badge, cost, End Session button - Active sessions: SplitPane with chat (45%) + terminal (55%), per-session storageKey - Ended sessions: read-only AgentChatPanel with history, no terminal, ended banner - Loading skeleton and error states (404 session-not-found + generic) - Error feedback when End Session fails - AgentChatPanel: readOnly + initialMessages props for ended session view - sessionsApi: getOne() and getMessages() endpoints - 19 unit tests covering all acceptance criteria
Contributor
WalkthroughConvert the sessions detail route to a server page that renders a new client component Changes
Sequence DiagramsequenceDiagram
participant User
participant ServerPage as Server: /sessions/[id]/page
participant Client as SessionDetailClient
participant API as sessionsApi
participant Chat as AgentChatPanel
participant Term as AgentTerminal
participant Split as SplitPane
participant Router as Router
User->>ServerPage: Request /sessions/{id}
ServerPage->>ServerPage: generateMetadata(params) -> title
ServerPage-->>User: Render page with SessionDetailClient(sessionId)
User->>Client: Client mounts with sessionId
Client->>API: GET /api/v2/sessions/{id}
API-->>Client: session metadata {id, state, ...}
alt state == active
Client->>Split: render SplitPane(storageKey=session-split-{id})
Client->>Chat: mount AgentChatPanel(sessionId, readOnly=false)
Client->>Term: mount AgentTerminal(sessionId)
loop poll while active
Client->>API: GET /api/v2/sessions/{id} (poll)
API-->>Client: updated metadata
end
User->>Client: Click "End Session"
Client->>API: DELETE /api/v2/sessions/{id}
API-->>Client: success
Client->>Router: push('/sessions')
else state == ended
Client->>API: GET /api/v2/sessions/{id}/messages
API-->>Client: ChatMessage[] history
Client->>Chat: render AgentChatPanel(readOnly=true, initialMessages)
Client-->>Term: do not mount AgentTerminal
end
alt fetch error (404)
API-->>Client: 404 error
Client-->>User: "Session not found" + back link
else other error
API-->>Client: error
Client-->>User: "Failed to load session"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Review: feat(web-ui): /sessions/[id] detail page (#509)This is a well-structured implementation. The TDD approach, SWR polling strategy, and active/ended session branching are all solid. A few items worth addressing: Bug:
|
- Remove 'use client' directive from test file (no-op in Jest) - Remove redundant inline height style (flex-1 handles this) - Use useRef flag for message history fetch (avoids dep array smell) - Fix conditional JSX wrapper formatting in AgentChatPanel - Add role='alert' to error message for a11y
Follow-up ReviewAll five issues flagged in the previous review have been addressed:
No new concerns. The SWR polling function, null-safe cost display, and Approving — ready to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/src/app/sessions/`[id]/page.tsx:
- Around line 73-80: The current useEffect swallowing fetch failures via
catch(() => setHistoryMessages([])) makes network/500 errors indistinguishable
from a genuinely empty transcript; update the load logic used with
useEffect/session?.state/historyMessages to track loading and error separately
(e.g., add historyLoading and historyError state), call
sessionsApi.getMessages(session.id).then(setHistoryMessages).catch(err =>
setHistoryError(err)) instead of setting an empty array on error, and ensure the
read-only rendering branch checks historyError to show an explicit failure state
rather than treating errors as an empty chat.
- Around line 83-95: The handleEndSession handler currently calls
sessionsApi.end immediately; add a user confirmation step (e.g., using
window.confirm or your app’s confirmation modal) before making the DELETE call
so a stray click won’t terminate the session. Specifically, inside
handleEndSession (before setEndingSession and sessionsApi.end(session.id))
prompt the user and return early if they cancel; keep existing error handling
and state updates (setEndError, setEndingSession, router.push) intact. Ensure
the check still respects session and session.state !== 'active' and include the
session id or a clear message in the confirmation prompt for clarity.
- Around line 56-60: The code currently treats any non-active session (checks
like !isActive) as ended which mislabels 'paused' sessions and removes live UI;
update the logic so polling and UI branches explicitly check session.state ===
'ended' where you currently stop polling or render the "ended" UI (notably in
the useSWR refreshInterval predicate and the places that render the ended
banner, hide the terminal, set chat to read-only, or disable the end button).
Concretely, change the refreshInterval handler in the useSWR call to only stop
polling when session.state === 'ended', replace other !isActive conditional
branches with explicit session.state === 'ended' checks, and add separate
handling for session.state === 'paused' to keep or present the appropriate
paused UI (e.g., a paused banner and preserving live terminal/chat behavior) in
the same components referenced around the useSWR call and the conditional blocks
that render the ended banner, terminal, chat read-only state, and end button.
In `@web-ui/src/components/sessions/AgentChatPanel.tsx`:
- Around line 199-209: AgentChatPanel is using live hook state from useAgentChat
even when readOnly is true, causing the header and empty-state to show
active-session chrome; modify the rendering logic to derive display values from
readOnly rather than the hook state: call useAgentChat(readOnly ? null :
sessionId) as-is but compute local vars like displayStatus, displayCost, and
emptyStateCopy (e.g., displayStatus = readOnly ? 'ended' or 'disconnected' and
displayCost = readOnly ? undefined or the stored session cost; emptyStateCopy
should come from initialMessages presence when readOnly) and use those vars in
the header and empty-state rendering so ended/read-only sessions don't show
connected/socket UI or interactive prompts. Ensure references include
AgentChatPanel, useAgentChat, state, status, costUsd, initialMessages, and
readOnly.
In `@web-ui/src/lib/api.ts`:
- Around line 674-687: The API mapper in the api.get(...) call currently narrows
the response to an inline DTO and drops per-message metadata (the mapping that
returns {id, role, content, createdAt}), so add a proper Message DTO in
web-ui/src/types/index.ts that includes all fields returned by /messages (e.g.,
id, role, content, created_at plus toolName, toolInput, metadata or any other
per-message fields), import that type into web-ui/src/lib/api.ts, update the
api.get<> generic to use the new DTO, and change the response.data.map(...)
normalization to preserve and normalize the full payload (mapping created_at ->
createdAt and passing through toolName/toolInput/metadata) so AgentChatPanel
receives the tool-call details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c799008e-20d1-4b7a-9469-2af72c057459
📒 Files selected for processing (4)
web-ui/src/__tests__/components/sessions/SessionDetailPage.test.tsxweb-ui/src/app/sessions/[id]/page.tsxweb-ui/src/components/sessions/AgentChatPanel.tsxweb-ui/src/lib/api.ts
Comment on lines
+56
to
+60
| const { data: session, isLoading, error } = useSWR<Session>( | ||
| sessionId ? `/api/v2/sessions/${sessionId}` : null, | ||
| () => sessionsApi.getOne(sessionId), | ||
| { refreshInterval: (data) => (data?.state === 'active' ? 5000 : 0) } | ||
| ); |
Contributor
There was a problem hiding this comment.
Handle paused separately from ended.
SessionState also includes 'paused', but every !isActive path here assumes the session is ended: polling stops, the ended banner renders, the terminal disappears, chat becomes read-only, and the end button is disabled. A paused session will therefore be mislabeled and lose its live UI. Branch on session.state === 'ended' explicitly, then define the paused behavior separately.
Also applies to: 145-145, 186-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/app/sessions/`[id]/page.tsx around lines 56 - 60, The code
currently treats any non-active session (checks like !isActive) as ended which
mislabels 'paused' sessions and removes live UI; update the logic so polling and
UI branches explicitly check session.state === 'ended' where you currently stop
polling or render the "ended" UI (notably in the useSWR refreshInterval
predicate and the places that render the ended banner, hide the terminal, set
chat to read-only, or disable the end button). Concretely, change the
refreshInterval handler in the useSWR call to only stop polling when
session.state === 'ended', replace other !isActive conditional branches with
explicit session.state === 'ended' checks, and add separate handling for
session.state === 'paused' to keep or present the appropriate paused UI (e.g., a
paused banner and preserving live terminal/chat behavior) in the same components
referenced around the useSWR call and the conditional blocks that render the
ended banner, terminal, chat read-only state, and end button.
Comment on lines
+73
to
+80
| // Load message history for ended sessions via REST | ||
| useEffect(() => { | ||
| if (session?.state === 'ended' && historyMessages === undefined) { | ||
| sessionsApi | ||
| .getMessages(session.id) | ||
| .then(setHistoryMessages) | ||
| .catch(() => setHistoryMessages([])); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t mask history-fetch failures as an empty transcript.
catch(() => setHistoryMessages([])) makes a network/500 failure indistinguishable from a session that genuinely has no messages. The ended-session path then renders a misleading empty chat instead of an explicit failure state. Track history loading/error separately and surface that in the read-only branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/app/sessions/`[id]/page.tsx around lines 73 - 80, The current
useEffect swallowing fetch failures via catch(() => setHistoryMessages([]))
makes network/500 errors indistinguishable from a genuinely empty transcript;
update the load logic used with useEffect/session?.state/historyMessages to
track loading and error separately (e.g., add historyLoading and historyError
state), call
sessionsApi.getMessages(session.id).then(setHistoryMessages).catch(err =>
setHistoryError(err)) instead of setting an empty array on error, and ensure the
read-only rendering branch checks historyError to show an explicit failure state
rather than treating errors as an empty chat.
Comment on lines
+83
to
+95
| const handleEndSession = useCallback(async () => { | ||
| if (!session || session.state !== 'active') return; | ||
| setEndingSession(true); | ||
| setEndError(null); | ||
| try { | ||
| await sessionsApi.end(session.id); | ||
| router.push('/sessions'); | ||
| } catch { | ||
| setEndError('Failed to end session. Please try again.'); | ||
| } finally { | ||
| setEndingSession(false); | ||
| } | ||
| }, [session, router]); |
Contributor
There was a problem hiding this comment.
Add the missing confirmation before sending DELETE.
This handler ends the session on the first click, but the PR objective calls for a confirmation step before terminating an active session. Please gate the request behind a confirm dialog so a stray click can’t end the session immediately.
Minimal fix
const handleEndSession = useCallback(async () => {
if (!session || session.state !== 'active') return;
+ if (!window.confirm(`End session #${shortId(session.id)}?`)) return;
setEndingSession(true);
setEndError(null);
try {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleEndSession = useCallback(async () => { | |
| if (!session || session.state !== 'active') return; | |
| setEndingSession(true); | |
| setEndError(null); | |
| try { | |
| await sessionsApi.end(session.id); | |
| router.push('/sessions'); | |
| } catch { | |
| setEndError('Failed to end session. Please try again.'); | |
| } finally { | |
| setEndingSession(false); | |
| } | |
| }, [session, router]); | |
| const handleEndSession = useCallback(async () => { | |
| if (!session || session.state !== 'active') return; | |
| if (!window.confirm(`End session #${shortId(session.id)}?`)) return; | |
| setEndingSession(true); | |
| setEndError(null); | |
| try { | |
| await sessionsApi.end(session.id); | |
| router.push('/sessions'); | |
| } catch { | |
| setEndError('Failed to end session. Please try again.'); | |
| } finally { | |
| setEndingSession(false); | |
| } | |
| }, [session, router]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/app/sessions/`[id]/page.tsx around lines 83 - 95, The
handleEndSession handler currently calls sessionsApi.end immediately; add a user
confirmation step (e.g., using window.confirm or your app’s confirmation modal)
before making the DELETE call so a stray click won’t terminate the session.
Specifically, inside handleEndSession (before setEndingSession and
sessionsApi.end(session.id)) prompt the user and return early if they cancel;
keep existing error handling and state updates (setEndError, setEndingSession,
router.push) intact. Ensure the check still respects session and session.state
!== 'active' and include the session id or a clear message in the confirmation
prompt for clarity.
Comment on lines
+199
to
+209
| export function AgentChatPanel({ | ||
| sessionId, | ||
| className, | ||
| readOnly = false, | ||
| initialMessages, | ||
| }: AgentChatPanelProps) { | ||
| const { state, sendMessage, interrupt } = useAgentChat( | ||
| readOnly ? null : sessionId | ||
| ); | ||
| const { status, costUsd } = state; | ||
| const messages = initialMessages ?? state.messages; |
Contributor
There was a problem hiding this comment.
Read-only mode still renders live-session chrome.
When readOnly is true, useAgentChat(null) returns the hook’s initial state. That makes the panel show the default green/connected status and $0.0000, and if initialMessages is still missing it also falls back to the interactive empty-state copy. Please branch the header/empty state for read-only mode so ended sessions don’t advertise a live websocket or prompt the user to start a conversation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/sessions/AgentChatPanel.tsx` around lines 199 - 209,
AgentChatPanel is using live hook state from useAgentChat even when readOnly is
true, causing the header and empty-state to show active-session chrome; modify
the rendering logic to derive display values from readOnly rather than the hook
state: call useAgentChat(readOnly ? null : sessionId) as-is but compute local
vars like displayStatus, displayCost, and emptyStateCopy (e.g., displayStatus =
readOnly ? 'ended' or 'disconnected' and displayCost = readOnly ? undefined or
the stored session cost; emptyStateCopy should come from initialMessages
presence when readOnly) and use those vars in the header and empty-state
rendering so ended/read-only sessions don't show connected/socket UI or
interactive prompts. Ensure references include AgentChatPanel, useAgentChat,
state, status, costUsd, initialMessages, and readOnly.
Comment on lines
+674
to
+687
| const response = await api.get< | ||
| Array<{ | ||
| id: string; | ||
| role: string; | ||
| content: string; | ||
| created_at: string; | ||
| }> | ||
| >(`/api/v2/sessions/${encodeURIComponent(id)}/messages`, { params }); | ||
| return response.data.map((m) => ({ | ||
| id: m.id, | ||
| role: m.role as ChatMessage['role'], | ||
| content: m.content, | ||
| createdAt: m.created_at, | ||
| })); |
Contributor
There was a problem hiding this comment.
Preserve message metadata in ended-session history.
This mapper narrows the REST payload down to { id, role, content, created_at }, but the /messages response also carries per-message metadata. AgentChatPanel uses toolName/toolInput for tool-call rows, so ended-session transcripts will lose those details here. Please move the response DTO into web-ui/src/types/index.ts and normalize the full payload instead of dropping metadata behind an inline anonymous type.
As per coding guidelines, "TypeScript types must be defined in web-ui/src/types/index.ts and API client functionality in web-ui/src/lib/api.ts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/lib/api.ts` around lines 674 - 687, The API mapper in the
api.get(...) call currently narrows the response to an inline DTO and drops
per-message metadata (the mapping that returns {id, role, content, createdAt}),
so add a proper Message DTO in web-ui/src/types/index.ts that includes all
fields returned by /messages (e.g., id, role, content, created_at plus toolName,
toolInput, metadata or any other per-message fields), import that type into
web-ui/src/lib/api.ts, update the api.get<> generic to use the new DTO, and
change the response.data.map(...) normalization to preserve and normalize the
full payload (mapping created_at -> createdAt and passing through
toolName/toolInput/metadata) so AgentChatPanel receives the tool-call details.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
web-ui/src/app/sessions/[id]/page.tsx (3)
85-97:⚠️ Potential issue | 🟠 MajorMissing confirmation before ending session.
The PR objectives state the End Session button should "confirm" before terminating. Currently, a single click immediately calls
DELETE /api/v2/sessions/{id}with no safeguard.Add a confirmation prompt
const handleEndSession = useCallback(async () => { if (!session || session.state !== 'active') return; + if (!window.confirm(`End session #${shortId(session.id)}?`)) return; setEndingSession(true); setEndError(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/sessions/`[id]/page.tsx around lines 85 - 97, The end-session flow in handleEndSession calls sessionsApi.end(session.id) immediately with no user confirmation; update handleEndSession to display a confirmation prompt (e.g., window.confirm or a modal) before calling sessionsApi.end, and only proceed to setEndingSession(true)/call sessionsApi.end/route to '/sessions' when the user confirms; ensure cancellation leaves state unchanged and still clears setEndingSession(false) in the finally block, and reference handleEndSession and sessionsApi.end when making the change.
74-83:⚠️ Potential issue | 🟠 MajorNetwork errors in message history fetch are silently swallowed.
The
.catch(() => setHistoryMessages([]))makes a 500 error indistinguishable from a genuinely empty session. Users see an empty chat with no indication that the history failed to load.Track loading/error states explicitly:
+ const [historyError, setHistoryError] = useState<string | null>(null); useEffect(() => { if (session?.state === 'ended' && !messagesFetchedRef.current) { messagesFetchedRef.current = true; sessionsApi .getMessages(session.id) .then(setHistoryMessages) - .catch(() => setHistoryMessages([])); + .catch((err) => { + setHistoryError(err?.detail ?? 'Failed to load message history'); + setHistoryMessages([]); + }); } }, [session]);Then surface
historyErrorin the read-onlyAgentChatPanelbranch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/sessions/`[id]/page.tsx around lines 74 - 83, The current useEffect swallowing network errors (sessionsApi.getMessages(...).catch(() => setHistoryMessages([]))) hides 500s; update the logic to track loading and error states by introducing a historyLoading/historyError state (or refs) and set historyLoading true before calling sessionsApi.getMessages(session.id), set historyLoading false and setHistoryMessages on success, and on catch set historyError with the caught error (not just an empty array) and leave historyMessages untouched or empty as appropriate; ensure messagesFetchedRef is still used to avoid duplicate fetches and pass historyError (and optionally historyLoading) down into the read-only AgentChatPanel branch so the UI can surface a clear failure message instead of silently showing an empty chat.
56-60:⚠️ Potential issue | 🟠 Major
pausedsessions are incorrectly treated asended.The
refreshIntervalcallback stops polling for any non-activestate. Combined withisActive = session.state === 'active'at line 147, apausedsession will:
- Stop polling (may or may not be desired)
- Display the "This session has ended" banner (incorrect)
- Hide the terminal and show read-only chat (incorrect)
Consider handling
pausedexplicitly:- { refreshInterval: (data) => (data?.state === 'active' ? 5000 : 0) } + { refreshInterval: (data) => (data?.state === 'active' || data?.state === 'paused' ? 5000 : 0) }And at line 147+:
const isActive = session.state === 'active'; + const isEnded = session.state === 'ended'; const sid = shortId(session.id);Then use
isEndedfor the banner (line 189) and read-only logic (line 208).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/sessions/`[id]/page.tsx around lines 56 - 60, The SWR refreshInterval currently treats any non-'active' session (including 'paused') as stopped and the UI sets isActive = session.state === 'active', causing paused sessions to be shown as ended; update the useSWR refreshInterval callback to only stop polling when state === 'ended' (i.e., return 5000 for 'active' and 'paused', 0 for 'ended'), then introduce explicit helpers like isPaused = session?.state === 'paused' and isEnded = session?.state === 'ended' (keep isActive = session?.state === 'active') and use isEnded for the "session has ended" banner and read-only/chat visibility logic instead of negating isActive.
🧹 Nitpick comments (1)
web-ui/src/app/sessions/[id]/page.tsx (1)
208-214: Consider showing a loading state while fetching message history.When
historyMessagesisundefined(still loading), theAgentChatPanelfalls back to an empty message list. Users briefly see an empty chat before history loads. A loading indicator or skeleton within the panel would improve perceived responsiveness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/sessions/`[id]/page.tsx around lines 208 - 214, The AgentChatPanel currently receives initialMessages={historyMessages} and shows an empty chat while historyMessages is undefined; update the rendering to detect when historyMessages is undefined and show a loading indicator/skeleton instead of immediately rendering AgentChatPanel, or extend AgentChatPanel to accept an isLoading prop and render its internal skeleton when isLoading is true; reference the historyMessages value and the AgentChatPanel component (initialMessages prop) and ensure the loading state is toggled until the fetched historyMessages becomes defined so users see a spinner/skeleton instead of a blank chat.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/src/app/sessions/`[id]/page.tsx:
- Around line 107-108: The 404 detection in the error handling for the session
page is using the wrong ApiError property; replace the check error?.status ===
404 with error?.status_code === 404 (or check both if you want to be defensive)
so the isNotFound computation in page.tsx correctly detects "Session not found"
errors; update the isNotFound assignment near the top of the page component that
currently references error?.status and keep the existing error?.detail check
intact.
---
Duplicate comments:
In `@web-ui/src/app/sessions/`[id]/page.tsx:
- Around line 85-97: The end-session flow in handleEndSession calls
sessionsApi.end(session.id) immediately with no user confirmation; update
handleEndSession to display a confirmation prompt (e.g., window.confirm or a
modal) before calling sessionsApi.end, and only proceed to
setEndingSession(true)/call sessionsApi.end/route to '/sessions' when the user
confirms; ensure cancellation leaves state unchanged and still clears
setEndingSession(false) in the finally block, and reference handleEndSession and
sessionsApi.end when making the change.
- Around line 74-83: The current useEffect swallowing network errors
(sessionsApi.getMessages(...).catch(() => setHistoryMessages([]))) hides 500s;
update the logic to track loading and error states by introducing a
historyLoading/historyError state (or refs) and set historyLoading true before
calling sessionsApi.getMessages(session.id), set historyLoading false and
setHistoryMessages on success, and on catch set historyError with the caught
error (not just an empty array) and leave historyMessages untouched or empty as
appropriate; ensure messagesFetchedRef is still used to avoid duplicate fetches
and pass historyError (and optionally historyLoading) down into the read-only
AgentChatPanel branch so the UI can surface a clear failure message instead of
silently showing an empty chat.
- Around line 56-60: The SWR refreshInterval currently treats any non-'active'
session (including 'paused') as stopped and the UI sets isActive = session.state
=== 'active', causing paused sessions to be shown as ended; update the useSWR
refreshInterval callback to only stop polling when state === 'ended' (i.e.,
return 5000 for 'active' and 'paused', 0 for 'ended'), then introduce explicit
helpers like isPaused = session?.state === 'paused' and isEnded = session?.state
=== 'ended' (keep isActive = session?.state === 'active') and use isEnded for
the "session has ended" banner and read-only/chat visibility logic instead of
negating isActive.
---
Nitpick comments:
In `@web-ui/src/app/sessions/`[id]/page.tsx:
- Around line 208-214: The AgentChatPanel currently receives
initialMessages={historyMessages} and shows an empty chat while historyMessages
is undefined; update the rendering to detect when historyMessages is undefined
and show a loading indicator/skeleton instead of immediately rendering
AgentChatPanel, or extend AgentChatPanel to accept an isLoading prop and render
its internal skeleton when isLoading is true; reference the historyMessages
value and the AgentChatPanel component (initialMessages prop) and ensure the
loading state is toggled until the fetched historyMessages becomes defined so
users see a spinner/skeleton instead of a blank chat.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ff2a45c-0dfd-4836-9635-19bfddda92cb
📒 Files selected for processing (3)
web-ui/src/__tests__/components/sessions/SessionDetailPage.test.tsxweb-ui/src/app/sessions/[id]/page.tsxweb-ui/src/components/sessions/AgentChatPanel.tsx
✅ Files skipped from review due to trivial changes (1)
- web-ui/src/tests/components/sessions/SessionDetailPage.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/src/components/sessions/AgentChatPanel.tsx
Comment on lines
+107
to
+108
| if (error || !session) { | ||
| const isNotFound = error?.status === 404 || error?.detail === 'Session not found'; |
Contributor
There was a problem hiding this comment.
404 detection uses wrong property name.
Per the API interceptor (see web-ui/src/lib/api.ts lines 99-110), the ApiError object has status_code, not status. The current check error?.status === 404 will always be undefined === 404 → false, so 404 errors will render as generic failures instead of "Session not found".
- const isNotFound = error?.status === 404 || error?.detail === 'Session not found';
+ const isNotFound = error?.status_code === 404 || error?.detail === 'Session not found';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (error || !session) { | |
| const isNotFound = error?.status === 404 || error?.detail === 'Session not found'; | |
| if (error || !session) { | |
| const isNotFound = error?.status_code === 404 || error?.detail === 'Session not found'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/app/sessions/`[id]/page.tsx around lines 107 - 108, The 404
detection in the error handling for the session page is using the wrong ApiError
property; replace the check error?.status === 404 with error?.status_code ===
404 (or check both if you want to be defensive) so the isNotFound computation in
page.tsx correctly detects "Session not found" errors; update the isNotFound
assignment near the top of the page component that currently references
error?.status and keep the existing error?.detail check intact.
…ic title metadata - page.tsx: server component exporting generateMetadata for dynamic page title - SessionDetailClient.tsx: client component with all interactive logic - Tests now import SessionDetailClient directly + test generateMetadata - 20 tests passing
Follow-up ReviewAll five issues from my previous review have been addressed:
A couple of small observations on the new structure (neither blocks merge): Good split: Extracting the interactive logic into Minor — SWR key vs fetcher mismatch: The SWR key is the URL string Test coverage: The Ready to merge. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
web-ui/src/app/sessions/[id]/SessionDetailClient.tsx (4)
102-103:⚠️ Potential issue | 🔴 CriticalUse
status_codefor the not-found branch.The API error shape here is
status_code, notstatus, so real 404s will fall through to the generic failure UI instead of “Session not found”. Please update this check and the matching test mocks together.Suggested fix
- const isNotFound = error?.status === 404 || error?.detail === 'Session not found'; + const isNotFound = error?.status_code === 404 || error?.detail === 'Session not found';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/sessions/`[id]/SessionDetailClient.tsx around lines 102 - 103, The not-found check in SessionDetailClient.tsx uses error?.status but the API returns error.status_code; update the condition in the isNotFound assignment to use error?.status_code === 404 (and keep the existing detail check), and update any unit/test mocks that assert or simulate error.status to use status_code instead so 404 error flows hit the “Session not found” branch; look for the isNotFound variable and related tests/mocks in this component and adjust them accordingly.
70-77:⚠️ Potential issue | 🟠 MajorSurface transcript-load failures instead of showing an empty chat.
catch(() => setHistoryMessages([]))makes a real/messagesfailure indistinguishable from “this session has no messages”. Because Line 72 flips the ref before the request, a transient failure also blocks any retry for the rest of the mount. Track loading/error separately and render that in the read-only branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/sessions/`[id]/SessionDetailClient.tsx around lines 70 - 77, The current useEffect flips messagesFetchedRef.current before calling sessionsApi.getMessages and swallows failures by calling catch(() => setHistoryMessages([])), which makes errors indistinguishable from an empty transcript and prevents retries; change the logic in the useEffect that references messagesFetchedRef, sessionsApi.getMessages, and setHistoryMessages to: delay setting messagesFetchedRef.current = true until after a successful response, introduce separate state (e.g., historyLoading and historyError) to track loading and error states, set historyLoading true before the request and false after, set historyError on failure instead of replacing messages with an empty array, and update the read-only render branch to show loading/error UI based on historyLoading/historyError so real failures are surfaced and retries are possible.
61-62:⚠️ Potential issue | 🟠 MajorDon’t collapse every non-
activestate into the ended UX.Polling, the disabled end button, the ended banner, and the read-only/chat-only layout all key off
session.state === 'active'. That makes any other state look “ended” and strips the live UI prematurely; branch onsession.state === 'ended'explicitly and handle the remaining states separately.Also applies to: 142-142, 175-176, 184-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/sessions/`[id]/SessionDetailClient.tsx around lines 61 - 62, The code treats any non-'active' session as "ended"; change the logic to explicitly check for session.state === 'ended' instead of negating 'active'. Update the refreshInterval callback (where refreshInterval: (data) => (data?.state === 'active' ? 5000 : 0)), the End button disabled logic, the ended banner render, and the read-only/chat-only layout branches in SessionDetailClient (they currently use session.state === 'active'); make them branch off session.state === 'ended' for the ended UX and handle other states (e.g., 'paused', 'pending', 'initializing') separately so they keep live UI where appropriate.
80-90:⚠️ Potential issue | 🟠 MajorConfirm before sending the destructive DELETE.
This still terminates the session on the first click, but the PR objective requires a confirmation step. Please gate the request behind a confirm dialog/modal and return early on cancel.
Minimal fix
const handleEndSession = useCallback(async () => { if (!session || session.state !== 'active') return; + if (!window.confirm(`End session #${shortId(session.id)}?`)) return; setEndingSession(true); setEndError(null); try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/sessions/`[id]/SessionDetailClient.tsx around lines 80 - 90, The handleEndSession function currently calls sessionsApi.end immediately; gate this destructive action by prompting the user and returning early on cancel. Update handleEndSession to show a confirmation dialog/modal (e.g., using window.confirm or the existing modal component) before calling sessionsApi.end(session.id); if the user cancels, do not call sessionsApi.end, do not setEndingSession, and simply return. Keep existing error handling (setEndError) and state updates (setEndingSession, router.push) unchanged for the confirmed path.
🧹 Nitpick comments (1)
web-ui/src/__tests__/components/sessions/SessionDetailPage.test.tsx (1)
24-39: Assert the ended-session transcript wiring, not just the read-only chrome.The requirement here is “load
/messagesintoAgentChatPanel”. Because the mock dropsinitialMessagesand none of the ended-session tests wait forsessionsApi.getMessages, this suite would stay green if the transcript fetch stopped working.Small coverage upgrade
jest.mock('@/components/sessions/AgentChatPanel', () => ({ AgentChatPanel: ({ sessionId, readOnly, + initialMessages, }: { sessionId: string; readOnly?: boolean; + initialMessages?: unknown[]; }) => ( <div data-testid="agent-chat-panel" data-session-id={sessionId} data-read-only={readOnly ? 'true' : 'false'} + data-initial-count={initialMessages?.length ?? -1} > {!readOnly && <textarea aria-label="Message input" />} </div> ), }));render(<SessionDetailClient sessionId={SESSION_ID} />); - expect(screen.getByTestId('agent-chat-panel')).toHaveAttribute('data-read-only', 'true'); + await waitFor(() => expect(mockSessApiGetMessages).toHaveBeenCalledWith(SESSION_ID)); + expect(screen.getByTestId('agent-chat-panel')).toHaveAttribute('data-read-only', 'true'); + expect(screen.getByTestId('agent-chat-panel')).toHaveAttribute('data-initial-count', '0');Also applies to: 226-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/__tests__/components/sessions/SessionDetailPage.test.tsx` around lines 24 - 39, The mock for AgentChatPanel in SessionDetailPage.test.tsx drops the initialMessages prop so the tests never verify that sessionsApi.getMessages data is wired into the panel; update the mock for AgentChatPanel to accept and render an initialMessages prop (or at least expose it via a data attribute) and then update the ended-session tests to wait for sessionsApi.getMessages to resolve and assert that the returned messages appear in the mocked AgentChatPanel (or that initialMessages equals the API response). Target the AgentChatPanel mock and the tests referencing sessionsApi.getMessages so the suite will fail if initialMessages/transcript wiring breaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web-ui/src/app/sessions/`[id]/SessionDetailClient.tsx:
- Around line 102-103: The not-found check in SessionDetailClient.tsx uses
error?.status but the API returns error.status_code; update the condition in the
isNotFound assignment to use error?.status_code === 404 (and keep the existing
detail check), and update any unit/test mocks that assert or simulate
error.status to use status_code instead so 404 error flows hit the “Session not
found” branch; look for the isNotFound variable and related tests/mocks in this
component and adjust them accordingly.
- Around line 70-77: The current useEffect flips messagesFetchedRef.current
before calling sessionsApi.getMessages and swallows failures by calling catch(()
=> setHistoryMessages([])), which makes errors indistinguishable from an empty
transcript and prevents retries; change the logic in the useEffect that
references messagesFetchedRef, sessionsApi.getMessages, and setHistoryMessages
to: delay setting messagesFetchedRef.current = true until after a successful
response, introduce separate state (e.g., historyLoading and historyError) to
track loading and error states, set historyLoading true before the request and
false after, set historyError on failure instead of replacing messages with an
empty array, and update the read-only render branch to show loading/error UI
based on historyLoading/historyError so real failures are surfaced and retries
are possible.
- Around line 61-62: The code treats any non-'active' session as "ended"; change
the logic to explicitly check for session.state === 'ended' instead of negating
'active'. Update the refreshInterval callback (where refreshInterval: (data) =>
(data?.state === 'active' ? 5000 : 0)), the End button disabled logic, the ended
banner render, and the read-only/chat-only layout branches in
SessionDetailClient (they currently use session.state === 'active'); make them
branch off session.state === 'ended' for the ended UX and handle other states
(e.g., 'paused', 'pending', 'initializing') separately so they keep live UI
where appropriate.
- Around line 80-90: The handleEndSession function currently calls
sessionsApi.end immediately; gate this destructive action by prompting the user
and returning early on cancel. Update handleEndSession to show a confirmation
dialog/modal (e.g., using window.confirm or the existing modal component) before
calling sessionsApi.end(session.id); if the user cancels, do not call
sessionsApi.end, do not setEndingSession, and simply return. Keep existing error
handling (setEndError) and state updates (setEndingSession, router.push)
unchanged for the confirmed path.
---
Nitpick comments:
In `@web-ui/src/__tests__/components/sessions/SessionDetailPage.test.tsx`:
- Around line 24-39: The mock for AgentChatPanel in SessionDetailPage.test.tsx
drops the initialMessages prop so the tests never verify that
sessionsApi.getMessages data is wired into the panel; update the mock for
AgentChatPanel to accept and render an initialMessages prop (or at least expose
it via a data attribute) and then update the ended-session tests to wait for
sessionsApi.getMessages to resolve and assert that the returned messages appear
in the mocked AgentChatPanel (or that initialMessages equals the API response).
Target the AgentChatPanel mock and the tests referencing sessionsApi.getMessages
so the suite will fail if initialMessages/transcript wiring breaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7300ec73-e1f8-4fed-8792-de9c979e563c
📒 Files selected for processing (3)
web-ui/src/__tests__/components/sessions/SessionDetailPage.test.tsxweb-ui/src/app/sessions/[id]/SessionDetailClient.tsxweb-ui/src/app/sessions/[id]/page.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements #509 — the final integration page that assembles all session UI components into a working interactive session view.
web-ui/src/app/sessions/[id]/page.tsxreplacing the stub with full implementationSplitPanewithAgentChatPanel(45%, left) +AgentTerminal(55%, right), per-sessionstorageKeyAgentChatPanelwith message history (REST fetch), no terminal, ended bannerAgentChatPanel: addedreadOnlyandinitialMessagesprops for ended session viewsessionsApi: addedgetOne(id)andgetMessages(id)to API clientAcceptance Criteria
AgentChatPanelis mounted and chat is functional for active sessionsAgentTerminalis mounted and terminal is functional for active sessionsSplitPanedivider is draggable; position persists per session/sessionsshows updated session stateTest Plan
cost_usdnull safetyImplementation Notes
AgentChatPanelowns cost state viauseAgentChatinternally; header shows REST metadata costGET /api/v2/sessions/{id}/messages(REST) on mount, passed asinitialMessagestoAgentChatPanelrefreshIntervalis a function — polls every 5s for active sessions, stops for ended sessionsreadOnlyprop: whentrue, passesnulltouseAgentChat(no WS connection) and hides input barCloses #509
Summary by CodeRabbit
New Features
Tests